Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up mk_GEOSldasRestarts and process_rst.csh #551

Merged
merged 25 commits into from
Sep 2, 2022

Conversation

weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang weiyuan-jiang commented May 10, 2022

Revise processing of GEOSldas restarts:

  • Adopt new GMAO_Shared utility remap_restarts.py.
  • Re-mapping of LDASsa-formatted restarts is no longer supported.
  • Update ldas_setup to python3.
  • Replace process_rst.csh with cleaned-up python script process_rst.py.

This branch should have zero-diff except:

  1. The regridded restart has non-zero TSURF.
  2. The regridded restart has the same set of variables as the original restart, i.e. , WW, FR, CQ, CH etc. are not added if the original restart does not have them.
  3. This branch depends on newer versions of GEOSgcm_GridComp and MAPL.

@gmao-rreichle gmao-rreichle marked this pull request as draft July 21, 2022 13:10
@gmao-rreichle
Copy link
Contributor

gmao-rreichle commented Jul 21, 2022

@weiyuan-jiang, @biljanaorescanin,
Now that Scott has merged GEOSgcm_GridComp PR#571 and
is in the process of merging GMAO_Shared PR#238, I'm wondering if we should merge this GEOSldas PR as is or tweak it further.
A quick inspection of the changes suggests that this PR calls mk_catchANDcnRestarts.F90 directly, rather than going through the new remap_catchANDcn.py. Would it make sense to further modify this PR to use the new python script?
In this context, I know Scott needs command line options for the new remap restarts python script(s) before he can use it in the GCM setup script. This means we'll have to do some additional work on the python scripts, and it may be better to wait until this new work is complete before we convert the LDAS setup to use the new remap restarts utilities. (In a nutshell, Scott needs a one-line command option to remap restarts, which isn't an option right now because everything goes through yaml files. I can't say for sure, but I don't think we need to abandon the yaml files. Rather, it should be possible to create a wrapper script or function that deals with creating the yaml file when given command line inputs.)
Thoughts?
PS: Has this PR been tested recently? I don't see a test report in the Conversation. Before we merge the PR, we should probably test it again after Scott completed his merger of GMAO_Shared PR#238.

@weiyuan-jiang
Copy link
Contributor Author

I tested this branch before but I will test it again after things are settled. Yes, I will add a command line to generate the yaml file.

@weiyuan-jiang
Copy link
Contributor Author

This PR ( may need to change component.yaml ) is zero diff except global assim run. This assim run used LDASsa's restart, which is not supported any more. We should change the restart.

@gmao-rreichle
Copy link
Contributor

The two most recent commits revert the components.yaml file back to the "develop" version, in prep for merging the PR into "develop".
This PR requires first merging of GEOS-ESM/GMAO_Shared#276

self.in_rstfile = '/discover/nobackup/projects/gmao/ssd/land/l_data/LandRestarts_for_Regridding' \
'/CatchCN/M09/20151231/catchcn_internal_rst'
self.in_tilefile = '/discover/nobackup/ltakacs/bcs/Heracles-NL/SMAP_EASEv2_M09/SMAP_EASEv2_M09_3856x1624.til'
elif (self.catch == 'catchcnclm45'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weiyuan-jiang, @gmao-jkolassa:
If I'm guessing right, the current ldas_setup does not have information about a dummy restart file for CatchCNCLM45. It looks like the dummy restart files for the other model versions are all in the M09 tile space. @gmao-jkolassa, do you have an M09 restart for CatchCNCLM45 that could be used as a dummy restart here? If not, we could use the M36 restart from the LDAS_GLOBALCNCLM45 nightly test.
@weiyuan-jiang, please work with @gmao-jkolassa to implement a dummy CatchCNCLM45 restart in ldas_setup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have an M09 restart file readily available, but I can generate one if that is preferred. Please let me know.

@weiyuan-jiang I will send you the path to an M36 restart that can be used if an M09 restart is not needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmao-jkolassa: I'll leave it up to you to decide if we need an M09 dummy restart or if the M36 restart from the nightly test case is sufficient. Note that this is just a dummy and meant to give users a start by providing a properly formatted restart. Users will need to do some spin-up (unless the dummy restart itself is spun up properly, in the same tile space, and for the desired restart date, which is not likely to ever happen).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is only meant to serve as a dummy restart with the understanding that some additional spin-up is necessary, I think it is fine to use the M36 restart file from the nightly test. I will work with @weiyuan-jiang to make sure it is copied to an accessible space.

@gmao-rreichle
Copy link
Contributor

PRs #569 and #574 should be merged first to minimize conflicts.
Prior to merging the present PR, need to change CMakeLists.txt in the present branch to make it compatible with changes in #569.

@gmao-rreichle
Copy link
Contributor

PRs #569 and #574 should be merged first to minimize conflicts. Prior to merging the present PR, need to change CMakeLists.txt in the present branch to make it compatible with changes in #569.

PRs #569 and #574 have been merged, and the present branch has been updated to "develop," incl. resolution of the conflict in CMakeLists.txt.

Successfully completed LDAS Nightly 0-diff tests (Intel only) after e6994a3 (i.e., before updating of CatchCN dummy restart files).

@biljanaorescanin
Copy link
Contributor

@gmao-rreichle all intel tests passed

@biljanaorescanin
Copy link
Contributor

biljanaorescanin commented Sep 2, 2022

@gmao-rreichle Since this is on develop and current develop fails after change we did for new GNU compiler, these tests fail for same tests. All intel tests passed.

gnuconus pass pass 12 min pass/FAIL -- / --
gnuglobal -- -- -- pass/FAIL pass/FAIL
gnuglobalcs -- -- -- pass/FAIL pass/FAIL
gnuglobalcnclm4 -- -- -- pass/FAIL -- / --
gnuglobalcnclm45 -- -- -- pass/FAIL -- / --

@gmao-rreichle gmao-rreichle marked this pull request as ready for review September 2, 2022 13:27
@gmao-rreichle gmao-rreichle requested a review from a team as a code owner September 2, 2022 13:27
@gmao-rreichle gmao-rreichle merged commit 34d08fd into develop Sep 2, 2022
@gmao-rreichle gmao-rreichle deleted the feature/wjiang/clean_mk_restarts branch September 2, 2022 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants